-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate On Save #340
Validate On Save #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate is not run on open, only on save. This makes it awkward when opening your project, you will not see any validation errors for the file in focus until you save.
I wouldn't say it's awkward, that's just the nature of how it works given the limitations we're working with. I have doubts that this behaviour will change any time soon due to the fact that validate
requires providers to be installed, which in turn introduces a lot of complexity, i.e. increases chance of validate
to fail for reasons unrelated to the configuration itself. As far as I know there's unfortunately a decent amount of users which start the editor without running init
and it wouldn't make sense to hit them with (even more) errors straight after opening the folder.
Relatedly I think we should probably detect uninitialized workspace - i.e. missing providers and either ignore validation entirely, or raise that as info or warning, certainly not as an error, but I'm totally happy for that to be addressed in a separate PR given that it's opt-in for now.
Modules is supported, but the experience isn't ideal. For simplicity, in #323 we do not run terraform validate when a module file is saved. This is because it could be referenced/downstream from multiple root modules (we don't know which root module to call terraform validate in? Could lead to weird duplicate diagnostics). terraform validate does of course provide diagnostics for any referenced modules when saving a file in a root module (great! as expected).
I'm not sure what the best UX here is either, but I think we should at least display diags which are actionable - i.e. diags from local modules which can be edited and fixed from where they're installed, since changes don't involve release, but can be committed and tested alongside the module which calls this submodule.
As for external modules (coming from GitHub or the Registry) I'm not sure. I guess it's useful to know whether the module works in a particular context with particular variable values, but I'm not sure how useful that is if it can't really be fixed from that context - i.e. users should not be editing external modules installed into .terraform
.
All that said I'd be happy to leave UX enhancements for a follow-up PR.
This overall LGTM aside from my two inline comments.
@@ -394,9 +394,44 @@ func (rm *rootModule) ExecuteTerraformValidate(ctx context.Context) (map[string] | |||
} | |||
|
|||
// an entry for each file should exist, even if there are no diags | |||
for filename := range rm.pFilesMap { | |||
for filename := range rm.parsedFiles() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Looks like this was previously prone to race conditions if we were parsing files (and hence writing into the map) while also reading from that map.
diagsMap[filename] = make(hcl.Diagnostics, 0) | ||
} | ||
// since validation applies to linked modules, create an entry for all | ||
// files of linked modules | ||
for _, m := range rm.moduleManifest.Records { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for reading the module manifest here? Could we not create the entries "lazily" automatically below? e.g. something like
diags, ok := diagsMap[d.Range.Filename]
if !ok {
diagsMap[d.Range.Filename] = make(hcl.Diagnostics, 0)
diags = diagsMap[d.Range.Filename]
}
I understand there is a need for filtering out submodule diags, but couldn't this be done via something like strings.HasPrefix(d.Range.Filename, rm.Path())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I don't believe so no, the point of this is to create an entry in the map for every possible file, so that clearing diagnostics over the protocol works correctly. I also authored this a while though so I can't describe it in detail, but we can revisit/refactor in follow ups
One more thing: Will you document the new config option under https://github.com/hashicorp/terraform-ls/blob/main/docs/SETTINGS.md please? |
Fix bug in validation when modules are present
35e1c06
to
ddb2f0c
Compare
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
This creates a new configuration object for the language server called
ExperimentalFeatures
which allows users to opt into specific features which may not be ready to be on by default. The first feature is callingterraform validate
on save (with the saved file as the target directory).This does work as expected, however the user experience is a bit awkward in 2 cases:
terraform validate
when a module file is saved. This is because it could be referenced/downstream from multiple root modules (we don't know which root module to call terraform validate in? Could lead to weird duplicate diagnostics).terraform validate
does of course provide diagnostics for any referenced modules when saving a file in a root module (great! as expected).I think with the right messaging, and the fact that the feature is opt-in, that this UX is okay for now, but could use improvement.
To be clear this clearing issue only affects saves of modules, not of regular files of "rootmodules" (side note we really need to fixup our nomenclature on this topic)
In contrast, when we enable calling validate from the command palette on the client side, we would ask the user to specify which folder to run validate in, from a quick-picker style dialog. This experience is smoother since I think intuitively the user will always pick the right spot and won't experience this awkward clearing deficiency.